Updates exception handling with FlowFrameworkException AND adds dryrun param to Create Workflow #137

joshpalis · 2023-10-31T22:23:40Z

Description

This PR updates the workflow parsing, workflow validation, workflow execution and rest action exception handling to throw FlowFrameworkExceptions with the correct REST Status code.

Note : I have opted not to throw FlowFrameworkExceptions for any of the workflow steps, since these steps are executed after we return a rest response back to the user. FlowFrameworkExceptions allow us to control the status of a rest response, since workflow step exceptions will be indexed into the user_outputs field of the State index, there's no need for a rest status for those cases

Example Create workflow request with missing create_connector_step :

curl -i -XPOST "localhost:9200/_plugins/_flow_framework/workflow" -H "Content-Type:application/json" --data '{"name":"deploy-register-model","description":"test case","use_case":"TEST_USE_CASE","version":{"template":"1.0.0","compatibility":["2.12.0","3.0.0"]},"workflows":{"provision":{"nodes":[{"id":"workflow_step_1","type":"register_model","user_inputs":{"name":"openAI-gpt-3.5-turbo","function_name":"remote","description":"test model"}},{"id":"workflow_step_2","type":"deploy_model","user_inputs":{}}],"edges":[{"source":"workflow_step_1","dest":"workflow_step_2"}]}}}'
HTTP/1.1 201 Created
X-OpenSearch-Version: OpenSearch/3.0.0-SNAPSHOT (opensearch)
content-type: application/json; charset=UTF-8
content-length: 38

{"workflow_id":"FuvPh4sBZkuHXaxzsBW4"}%

Attempt to provision will fail due to graph validation, response is returned in the correct format with the correct rest status :

curl -i -XPOST "localhost:9200/_plugins/_flow_framework/workflow/FuvPh4sBZkuHXaxzsBW4/_provision?pretty"
HTTP/1.1 400 Bad Request
X-OpenSearch-Version: OpenSearch/3.0.0-SNAPSHOT (opensearch)
content-type: application/json; charset=UTF-8
content-length: 121

{
  "error" : "Request failed with exception: [Invalid graph, missing the following required inputs : [connector_id]]"
}

OpenSearch logs that graph validation failed :

[2023-10-31T22:16:57,149][INFO ][o.o.p.PluginsService     ] [dev-dsk-jpalis-2c-27c8aa11.us-west-2.amazon.com] PluginService:onIndexModule index:[.plugins-ai-global-context/aKVyVY6yTlad7ns_OxmMPg]
[2023-10-31T22:16:57,171][INFO ][o.o.c.m.MetadataCreateIndexService] [dev-dsk-jpalis-2c-27c8aa11.us-west-2.amazon.com] [.plugins-ai-global-context] creating index, cause [api], templates [], shards [1]/[1]
[2023-10-31T22:16:57,172][INFO ][o.o.c.r.a.AllocationService] [dev-dsk-jpalis-2c-27c8aa11.us-west-2.amazon.com] updating number_of_replicas to [0] for indices [.plugins-ai-global-context]
[2023-10-31T22:16:57,198][INFO ][o.o.p.PluginsService     ] [dev-dsk-jpalis-2c-27c8aa11.us-west-2.amazon.com] PluginService:onIndexModule index:[.plugins-ai-global-context/aKVyVY6yTlad7ns_OxmMPg]
[2023-10-31T22:16:57,249][INFO ][o.o.c.r.a.AllocationService] [dev-dsk-jpalis-2c-27c8aa11.us-west-2.amazon.com] Cluster health status changed from [YELLOW] to [GREEN] (reason: [shards started [[.plugins-ai-global-context][0]]]).
[2023-10-31T22:16:57,270][INFO ][o.o.f.i.FlowFrameworkIndicesHandler] [dev-dsk-jpalis-2c-27c8aa11.us-west-2.amazon.com] create index:.plugins-ai-global-context
[2023-10-31T22:16:57,305][INFO ][o.o.p.PluginsService     ] [dev-dsk-jpalis-2c-27c8aa11.us-west-2.amazon.com] PluginService:onIndexModule index:[.plugins-workflow-state/QTgdVC7WS3qHY6vYwQE8Vw]
[2023-10-31T22:16:57,314][INFO ][o.o.c.m.MetadataCreateIndexService] [dev-dsk-jpalis-2c-27c8aa11.us-west-2.amazon.com] [.plugins-workflow-state] creating index, cause [api], templates [], shards [1]/[1]
[2023-10-31T22:16:57,314][INFO ][o.o.c.r.a.AllocationService] [dev-dsk-jpalis-2c-27c8aa11.us-west-2.amazon.com] updating number_of_replicas to [0] for indices [.plugins-workflow-state]
[2023-10-31T22:16:57,335][INFO ][o.o.p.PluginsService     ] [dev-dsk-jpalis-2c-27c8aa11.us-west-2.amazon.com] PluginService:onIndexModule index:[.plugins-workflow-state/QTgdVC7WS3qHY6vYwQE8Vw]
[2023-10-31T22:16:57,379][INFO ][o.o.c.r.a.AllocationService] [dev-dsk-jpalis-2c-27c8aa11.us-west-2.amazon.com] Cluster health status changed from [YELLOW] to [GREEN] (reason: [shards started [[.plugins-workflow-state][0]]]).
[2023-10-31T22:16:57,397][INFO ][o.o.f.i.FlowFrameworkIndicesHandler] [dev-dsk-jpalis-2c-27c8aa11.us-west-2.amazon.com] create index:.plugins-workflow-state
[2023-10-31T22:16:57,409][INFO ][o.o.f.t.CreateWorkflowTransportAction] [dev-dsk-jpalis-2c-27c8aa11.us-west-2.amazon.com] create state workflow doc
[2023-10-31T22:17:20,174][ERROR][o.o.f.t.ProvisionWorkflowTransportAction] [dev-dsk-jpalis-2c-27c8aa11.us-west-2.amazon.com] Workflow validation failed for workflow : FuvPh4sBZkuHXaxzsBW4

Issues Resolved

Part of #88
#99

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

…user outputs Signed-off-by: Joshua Palis <[email protected]>

…s into previous_node_inputs and user_inputs, adds graph validation after topologically sorting a workflow into a list of ProcessNode Signed-off-by: Joshua Palis <[email protected]>

Signed-off-by: Joshua Palis <[email protected]>

…async, adding success test case for graph validation Signed-off-by: Joshua Palis <[email protected]>

Signed-off-by: Joshua Palis <[email protected]>

…ass and using constants Signed-off-by: Joshua Palis <[email protected]>

Signed-off-by: Joshua Palis <[email protected]>

…ctions to create flow framework exceptions Signed-off-by: Joshua Palis <[email protected]>

Signed-off-by: Joshua Palis <[email protected]>

codecov · 2023-10-31T22:27:27Z

Codecov Report

Merging #137 (6b364c6) into main (ac76a44) will decrease coverage by 1.97%.
The diff coverage is 31.20%.

@@             Coverage Diff              @@
##               main     #137      +/-   ##
============================================
- Coverage     69.05%   67.09%   -1.97%     
- Complexity      302      311       +9     
============================================
  Files            37       37              
  Lines          1393     1468      +75     
  Branches        132      139       +7     
============================================
+ Hits            962      985      +23     
- Misses          386      434      +48     
- Partials         45       49       +4

Files	Coverage Δ
...g/opensearch/flowframework/common/CommonValue.java	`100.00% <ø> (ø)`
...lowframework/exception/FlowFrameworkException.java	`45.45% <100.00%> (+5.45%)`	⬆️
...earch/flowframework/transport/WorkflowRequest.java	`100.00% <100.00%> (ø)`
...a/org/opensearch/flowframework/model/Template.java	`89.36% <0.00%> (ø)`
.../flowframework/workflow/WorkflowProcessSorter.java	`97.82% <87.50%> (+0.02%)`	⬆️
...rk/transport/ProvisionWorkflowTransportAction.java	`49.27% <33.33%> (ø)`
...lowframework/rest/RestProvisionWorkflowAction.java	`48.00% <35.00%> (-24.73%)`	⬇️
...h/flowframework/rest/RestCreateWorkflowAction.java	`44.00% <30.00%> (-22.67%)`	⬇️
...ework/transport/CreateWorkflowTransportAction.java	`45.71% <29.62%> (-9.61%)`	⬇️
...framework/indices/FlowFrameworkIndicesHandler.java	`29.18% <2.85%> (-2.39%)`	⬇️

Signed-off-by: Joshua Palis <[email protected]>

src/main/java/org/opensearch/flowframework/rest/RestCreateWorkflowAction.java

src/main/java/org/opensearch/flowframework/rest/RestProvisionWorkflowAction.java

…ed on exceptions thrown by the transport client Signed-off-by: Joshua Palis <[email protected]>

src/main/java/org/opensearch/flowframework/indices/FlowFrameworkIndicesHandler.java

src/main/java/org/opensearch/flowframework/rest/RestProvisionWorkflowAction.java

owaiskazi19

Looks good overall. General comment. Concatenate the error message with e.getMessage() in FlowFrameworkException.

…ore saving Signed-off-by: Joshua Palis <[email protected]>

joshpalis · 2023-11-02T21:49:20Z

I've actioned #99 as well in this PR.
This adds a dryrun param to the create workflow API, which if true, attempts to validate each workflow before saving the template.

Example create workflow request with a cyclical graph

curl -i -XPOST "localhost:9200/_plugins/_flow_framework/workflow?dryrun=true" -H "Content-Type:application/json" --data '{"name":"create-connector-register-deploy-model","description":"test case","use_case":"TEST_CASE","version":{"template":"1.0.0","compatibility":["2.12.0","3.0.0"]},"workflows":{"provision":{"nodes":[{"id":"workflow_step_1","type":"create_connector","user_inputs":{"name":"OpenAI Chat Connector","description":"The connector to public OpenAI model service for GPT 3.5","version":"1","protocol":"http","parameters":{"endpoint":"api.openai.com","model":"gpt-3.5-turbo"},"credential":{"openAI_key":"..."},"actions":[{"action_type":"predict","method":"POST","url":"https://${parameters.endpoint}/v1/chat/completions"}]}},{"id":"workflow_step_2","type":"register_model","previous_node_inputs":{"workflow_step_1":"connector_id"},"user_inputs":{"name":"openAI-gpt-3.5-turbo","function_name":"remote","description":"test model"}},{"id":"workflow_step_3","type":"deploy_model","previous_node_inputs":{"workflow_step_2":"model_id"}}],"edges":[{"source":"workflow_step_1","dest":"workflow_step_2"},{"source":"workflow_step_2","dest":"workflow_step_3"},{"source":"workflow_step_3","dest":"workflow_step_1"}]}}}'
HTTP/1.1 400 Bad Request
X-OpenSearch-Version: OpenSearch/3.0.0-SNAPSHOT (opensearch)
content-type: application/json; charset=UTF-8
content-length: 98

{"error":"Request failed with exception: [No start node detected: all nodes have a predecessor.]"}

src/main/java/org/opensearch/flowframework/common/CommonValue.java

src/main/java/org/opensearch/flowframework/transport/CreateWorkflowTransportAction.java

Signed-off-by: Joshua Palis <[email protected]>

owaiskazi19

LGTM! One suggestion is to keep the PR shorter and making it easy for review.

dbwiddis

Great work!

We're pretty inconsistent with our error messages. Not sure if we're too far gone to do anything about it though!

src/main/java/org/opensearch/flowframework/common/CommonValue.java

src/main/java/org/opensearch/flowframework/exception/FlowFrameworkException.java

Signed-off-by: Joshua Palis <[email protected]>

…n param to Create Workflow (#137) * Simplifying Template format, removing operations, resources created, user outputs Signed-off-by: Joshua Palis <[email protected]> * Initial commit, modifies use case template to seperate workflow inputs into previous_node_inputs and user_inputs, adds graph validation after topologically sorting a workflow into a list of ProcessNode Signed-off-by: Joshua Palis <[email protected]> * Adding tests Signed-off-by: Joshua Palis <[email protected]> * Adding validate graph test Signed-off-by: Joshua Palis <[email protected]> * Addressing PR comments, moving sorting/validating prior to executing async, adding success test case for graph validation Signed-off-by: Joshua Palis <[email protected]> * Adding javadocs Signed-off-by: Joshua Palis <[email protected]> * Moving validation prior to updating workflow state to provisioning Signed-off-by: Joshua Palis <[email protected]> * Addressing PR comments Part 1 Signed-off-by: Joshua Palis <[email protected]> * Addressing PR comments Part 2 : Moving field names to common value class and using constants Signed-off-by: Joshua Palis <[email protected]> * Adding definition for noop workflow step Signed-off-by: Joshua Palis <[email protected]> * Addressing PR comments Part 3 Signed-off-by: Joshua Palis <[email protected]> * Modifies rest actions to throw flow framework exceptions, transport actions to create flow framework exceptions Signed-off-by: Joshua Palis <[email protected]> * Fixing credentials field in workflow-step json Signed-off-by: Joshua Palis <[email protected]> * Fixing test Signed-off-by: Joshua Palis <[email protected]> * Using ExceptionsHelper.status() to determine the rest status code based on exceptions thrown by the transport client Signed-off-by: Joshua Palis <[email protected]> * Adding dryrun param to create workflow API, allows for validation before saving Signed-off-by: Joshua Palis <[email protected]> * concatenating log message with exception message on failure Signed-off-by: Joshua Palis <[email protected]> * Adding dry run test Signed-off-by: Joshua Palis <[email protected]> * Simplifying FlowFrameworkException::toXContent Signed-off-by: Joshua Palis <[email protected]> --------- Signed-off-by: Joshua Palis <[email protected]> (cherry picked from commit c547658) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

… AND adds dryrun param to Create Workflow (#144) Updates exception handling with FlowFrameworkException AND adds dryrun param to Create Workflow (#137) * Simplifying Template format, removing operations, resources created, user outputs * Initial commit, modifies use case template to seperate workflow inputs into previous_node_inputs and user_inputs, adds graph validation after topologically sorting a workflow into a list of ProcessNode * Adding tests * Adding validate graph test * Addressing PR comments, moving sorting/validating prior to executing async, adding success test case for graph validation * Adding javadocs * Moving validation prior to updating workflow state to provisioning * Addressing PR comments Part 1 * Addressing PR comments Part 2 : Moving field names to common value class and using constants * Adding definition for noop workflow step * Addressing PR comments Part 3 * Modifies rest actions to throw flow framework exceptions, transport actions to create flow framework exceptions * Fixing credentials field in workflow-step json * Fixing test * Using ExceptionsHelper.status() to determine the rest status code based on exceptions thrown by the transport client * Adding dryrun param to create workflow API, allows for validation before saving * concatenating log message with exception message on failure * Adding dry run test * Simplifying FlowFrameworkException::toXContent --------- (cherry picked from commit c547658) Signed-off-by: Joshua Palis <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

joshpalis added 19 commits October 25, 2023 20:02

Simplifying Template format, removing operations, resources created, …

5152ad3

…user outputs Signed-off-by: Joshua Palis <[email protected]>

Initial commit, modifies use case template to seperate workflow input…

6ddd3b2

…s into previous_node_inputs and user_inputs, adds graph validation after topologically sorting a workflow into a list of ProcessNode Signed-off-by: Joshua Palis <[email protected]>

merge main

3a382fe

Signed-off-by: Joshua Palis <[email protected]>

Adding tests

e116c10

Signed-off-by: Joshua Palis <[email protected]>

Adding validate graph test

becc510

Signed-off-by: Joshua Palis <[email protected]>

Added model-group step to workflow-steps.json, rebasing with main

52be80c

Signed-off-by: Joshua Palis <[email protected]>

Addressing PR comments, moving sorting/validating prior to executing …

ee8f3cb

…async, adding success test case for graph validation Signed-off-by: Joshua Palis <[email protected]>

Merge branch 'main' into validate

290cd06

Adding javadocs

67e993f

Signed-off-by: Joshua Palis <[email protected]>

Moving validation prior to updating workflow state to provisioning

ddf946f

Signed-off-by: Joshua Palis <[email protected]>

Addressing PR comments Part 1

63524fa

Signed-off-by: Joshua Palis <[email protected]>

Addressing PR comments Part 2 : Moving field names to common value cl…

56865d2

…ass and using constants Signed-off-by: Joshua Palis <[email protected]>

Merge branch 'main' into validate

250c4d4

Merge branch 'main' into validate

2c61f59

Adding definition for noop workflow step

eb428de

Signed-off-by: Joshua Palis <[email protected]>

Merge branch 'main' into validate

b8b01f5

Addressing PR comments Part 3

8ba9d0b

Signed-off-by: Joshua Palis <[email protected]>

Modifies rest actions to throw flow framework exceptions, transport a…

8163655

…ctions to create flow framework exceptions Signed-off-by: Joshua Palis <[email protected]>

merge main

217b018

Signed-off-by: Joshua Palis <[email protected]>

joshpalis added the backport 2.x backport PRs to 2.x branch label Oct 31, 2023

joshpalis requested review from dbwiddis, owaiskazi19, ohltyler, amitgalitz and jackiehanyang as code owners October 31, 2023 22:23

joshpalis mentioned this pull request Oct 31, 2023

Updates exception handling with FlowFrameworkException and fix Global Context mapping #106

Closed

joshpalis added 2 commits November 1, 2023 20:12

Fixing credentials field in workflow-step json

98ab980

Signed-off-by: Joshua Palis <[email protected]>

Fixing test

b3336b2

Signed-off-by: Joshua Palis <[email protected]>

joshpalis mentioned this pull request Nov 1, 2023

Search APIs to fetch all the workflows present #139

Merged

amitgalitz reviewed Nov 2, 2023

View reviewed changes

src/main/java/org/opensearch/flowframework/rest/RestCreateWorkflowAction.java Show resolved Hide resolved

amitgalitz reviewed Nov 2, 2023

View reviewed changes

src/main/java/org/opensearch/flowframework/rest/RestProvisionWorkflowAction.java Show resolved Hide resolved

dbwiddis mentioned this pull request Nov 2, 2023

Put plugin API behind a feature flag #142

Merged

Using ExceptionsHelper.status() to determine the rest status code bas…

515507a

…ed on exceptions thrown by the transport client Signed-off-by: Joshua Palis <[email protected]>

amitgalitz reviewed Nov 2, 2023

View reviewed changes

src/main/java/org/opensearch/flowframework/indices/FlowFrameworkIndicesHandler.java Outdated Show resolved Hide resolved

amitgalitz reviewed Nov 2, 2023

View reviewed changes

src/main/java/org/opensearch/flowframework/rest/RestProvisionWorkflowAction.java Show resolved Hide resolved

owaiskazi19 reviewed Nov 2, 2023

View reviewed changes

Adding dryrun param to create workflow API, allows for validation bef…

eefe602

…ore saving Signed-off-by: Joshua Palis <[email protected]>

joshpalis changed the title ~~Updates exception handling with FlowFrameworkException~~ Updates exception handling with FlowFrameworkException AND adds dryrun param to Create Workflow Nov 2, 2023

owaiskazi19 reviewed Nov 2, 2023

View reviewed changes

src/main/java/org/opensearch/flowframework/common/CommonValue.java Show resolved Hide resolved

src/main/java/org/opensearch/flowframework/transport/CreateWorkflowTransportAction.java Show resolved Hide resolved

joshpalis added 2 commits November 2, 2023 22:11

concatenating log message with exception message on failure

3021b3d

Signed-off-by: Joshua Palis <[email protected]>

Adding dry run test

a72904a

Signed-off-by: Joshua Palis <[email protected]>

owaiskazi19 approved these changes Nov 2, 2023

View reviewed changes

dbwiddis approved these changes Nov 3, 2023

View reviewed changes

src/main/java/org/opensearch/flowframework/common/CommonValue.java Show resolved Hide resolved

src/main/java/org/opensearch/flowframework/exception/FlowFrameworkException.java Outdated Show resolved Hide resolved

Simplifying FlowFrameworkException::toXContent

6b364c6

Signed-off-by: Joshua Palis <[email protected]>

joshpalis merged commit c547658 into opensearch-project:main Nov 3, 2023
20 of 21 checks passed

opensearch-trigger-bot bot mentioned this pull request Nov 3, 2023

[Backport 2.x] Updates exception handling with FlowFrameworkException AND adds dryrun param to Create Workflow #144

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates exception handling with FlowFrameworkException AND adds dryrun param to Create Workflow #137

Updates exception handling with FlowFrameworkException AND adds dryrun param to Create Workflow #137

joshpalis commented Oct 31, 2023 •

edited

Loading

codecov bot commented Oct 31, 2023 •

edited

Loading

owaiskazi19 left a comment

joshpalis commented Nov 2, 2023

owaiskazi19 left a comment

dbwiddis left a comment

Updates exception handling with FlowFrameworkException AND adds dryrun param to Create Workflow #137

Updates exception handling with FlowFrameworkException AND adds dryrun param to Create Workflow #137

Conversation

joshpalis commented Oct 31, 2023 • edited Loading

Description

Issues Resolved

codecov bot commented Oct 31, 2023 • edited Loading

Codecov Report

owaiskazi19 left a comment

Choose a reason for hiding this comment

joshpalis commented Nov 2, 2023

owaiskazi19 left a comment

Choose a reason for hiding this comment

dbwiddis left a comment

Choose a reason for hiding this comment

joshpalis commented Oct 31, 2023 •

edited

Loading

codecov bot commented Oct 31, 2023 •

edited

Loading